[temp] feat: add detail taxonomy page #5

rpenido · 2023-10-20T18:33:07Z

Supporting Information

Closes [Tagging] "Bare Bones" Taxonomy Detail Page openedx/modular-learning#138
Depends on:
- feat: Taxonomy export menu [FC-0036] openedx/frontend-app-authoring#645

Private-ref: FAL-3529

pomegranited

I've run this on my devstack and it's looking great so far, @rpenido !

I've left a few suggestions, mainly around how the DataTable component is configured.

And as you've noted, there's some copy-paste code in here which should be factored out and moved up under src/taxonomy somewhere.

src/index.jsx

src/taxonomy/api/hooks/api.js

src/taxonomy/taxonomy-detail/TagListTable.jsx

src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx

src/taxonomy/taxonomy-card/TaxonomyCard.jsx

src/taxonomy/taxonomy-card/TaxonomyCard.scss

src/index.jsx

codecov · 2023-10-25T16:58:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (chris/FAL-3528-taxonomy-export-menu@080b03e). Click here to learn what that means.

Additional details and impacted files

@@                          Coverage Diff                           @@
##             chris/FAL-3528-taxonomy-export-menu       #5   +/-   ##
======================================================================
  Coverage                                       ?   88.53%           
======================================================================
  Files                                          ?      435           
  Lines                                          ?     6679           
  Branches                                       ?     1432           
======================================================================
  Hits                                           ?     5913           
  Misses                                         ?      741           
  Partials                                       ?       25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pomegranited

These latest changes look great, @rpenido ! I found some issues with the REST API being hit more than we need, but your addition of TaxonomyLayout, and your taxonomy-detail components look great.

src/taxonomy/taxonomy-detail/TagListTable.jsx

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx

src/taxonomy/taxonomy-detail/TagListTable.jsx

src/index.jsx

bradenmacdonald · 2023-10-26T20:49:45Z

Here is the PR that will merge soon and affect how taxonomy data is returned from the API - hopefully making it more flexible for you: openedx/edx-platform#33553

pomegranited

Hi @rpenido there's a bug with the Export menu item, and I noted one optional nit.

But everything else is working great, so I'll approve pending that fix, and then it's ready for upstream review.

👍

I tested this on my devstack.
I read through the code
I checked for accessibility issues by using my keyboard to navigate the new and modified components.
~~Includes documentation~~ N/A
Commit structure follows OEP-0051

src/taxonomy/TaxonomyLayout.jsx

src/taxonomy/taxonomy-card/TaxonomyCardMenu.jsx

xitij2000 · 2023-10-31T10:18:53Z

src/taxonomy/TaxonomyLayout.scss

@@ -0,0 +1,3 @@
+.taxonomy-layout {
+  background-color: #E9E6E4;


This is equivalent to bg-light-400, so you can avoid the custom css.

Thank you! Fixed here: a12f262

xitij2000 · 2023-10-31T10:19:57Z

src/taxonomy/TaxonomyLayout.test.jsx

+    expect(getByTestId('mock-content')).toBeInTheDocument();
+    expect(getByTestId('mock-footer')).toBeInTheDocument();
+  });
+});


I don't think this test is adding anything. There is no logic in this component so I think this file can be entirely skipped.

It only helps the code coverage. If this metric is not an issue here, let me know, and I remove it!

xitij2000 · 2023-10-31T10:27:25Z

src/taxonomy/taxonomy-detail/TagListTable.jsx

+  const { tagList } = useTagListData(taxonomyId);
+
+  if (!tagList || !tagList.results) {
+    return 'Loading...';


I think you could use the Paragon Spinner instead

This code is outdated. I'm using the Loading status from the DataTable

bradenmacdonald · 2023-11-03T17:08:41Z

@rpenido @xitij2000 What's the status of this PR? And how does it relate to openedx#655 ?

rpenido · 2023-11-03T19:15:52Z

@rpenido @xitij2000 What's the status of this PR? And how does it relate to openedx#655 ?

The branch of this feature is based on feat: Taxonomy export menu [FC-0036] openedx/frontend-app-authoring#645 branch.
The feat: bare bones taxonomy detail page [FC-0036] openedx/frontend-app-authoring#655 is against master and is the official PR.

This is a temp PR against that branch to allow us to review the changes before openedx#645 is merged.

I'm waiting for openedx#645 approval to rebase this branch and bring the improvements from the review to here.

…x#647) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

openedx#650) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…x#641) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

pomegranited

👍 Thanks for that @rpenido ! This is good for upstream review now. CC @bradenmacdonald

I tested this using the REST API on my devstack
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation
Commit structure follows OEP-0051

bradenmacdonald · 2023-11-07T18:29:12Z

I guess this is blocked on openedx#645 now ? Once that PR is merged, please open a new PR for this on the upstream repo.

…openedx#651) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

rpenido · 2023-11-07T20:42:06Z

I guess this is blocked on openedx#645 now ? Once that PR is merged, please open a new PR for this on the upstream repo.

Sure!
The PR on upstream is here: openedx#655. I will rebase as soon as we get openedx#645 merged.

…v1.175.1 (openedx#663) * fix(deps): update dependency @edx/frontend-lib-content-components to v1.175.1 * fix: upgrade paragon --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: KristinAoki <[email protected]>

This toggle does nothing if an LTI tool is selected. We should hide it in that case.

ChrisChV · 2023-11-13T21:15:18Z

src/taxonomy/tag-list/data/selectors.js

@@ -0,0 +1,42 @@
+// @ts-check


@rpenido From what I understand from here, these would not be considered selectors in the context of the other MFE modules. To avoid colliding with that context is why I have placed functions like this in apiHooks.js. But you can wait until openedx#645 is approved to make those changes

ChrisChV

@rpenido Looks good 👍 Check my comments, but I think you can wait until openedx#645 is merged to do any change.

I tested this: I follow the testing instructions on feat: bare bones taxonomy detail page [FC-0036] openedx/frontend-app-authoring#655
I read through the code and considered the security, stability and performance implications of the changes.
I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
Includes tests for bugfixes and/or features added.
Includes documentation

src/taxonomy/taxonomy-detail/TaxonomyDetailMenu.jsx

src/taxonomy/data/types.mjs

* feat: System-defined tooltip added * feat: Taxonomy card menu added. Export menu item added * feat: Modal for export taxonomy * feat: Connect with export API * test: Tests for API and selectors * feat: Use windows.location.href to call the export endpoint * test: ExportModal.test added * style: Delete unnecessary code * docs: README updated with taxonomy feature * style: TaxonomyCard updated to a better code style * style: injectIntl replaced by useIntl on taxonomy pages and components * refactor: Move and rename taxonomy UI components to match 0002 ADR * refactor: Move api to data to match with 0002 ADR * test: Refactor ExportModal tests * chore: Fix validations * chore: Lint * refactor: Moving hooks to apiHooks * style: Nit on return null --------- Co-authored-by: Rômulo Penido <[email protected]> Co-authored-by: Christofer Chavez <[email protected]>

src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…x#676) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…#678) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…l-page

and fix tests to mock useQuery.

pomegranited · 2023-11-16T00:50:46Z

Closed in favor of openedx#655

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 2520d22 to a52cc85 Compare October 20, 2023 18:36

ChrisChV force-pushed the chris/FAL-3528-taxonomy-export-menu branch from 667bf1e to 09aa8b6 Compare October 20, 2023 18:45

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 6 times, most recently from b3f724e to 3d45969 Compare October 24, 2023 22:00

pomegranited requested changes Oct 25, 2023

View reviewed changes

pomegranited reviewed Oct 25, 2023

View reviewed changes

src/index.jsx Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 3 times, most recently from a7d5cd7 to 3d6b0df Compare October 25, 2023 16:12

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 5d927a9 to 87808bb Compare October 25, 2023 19:18

pomegranited requested changes Oct 26, 2023

View reviewed changes

src/taxonomy/taxonomy-detail/TagListTable.jsx Outdated Show resolved Hide resolved

src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx Show resolved Hide resolved

src/taxonomy/taxonomy-detail/TagListTable.jsx Outdated Show resolved Hide resolved

src/index.jsx Outdated Show resolved Hide resolved

pomegranited mentioned this pull request Oct 26, 2023

Tagging: More powerful, flexible implementation of get_filtered_tags() [FC-0036] openedx/openedx-learning#92

Merged

4 tasks

pomegranited approved these changes Oct 30, 2023

View reviewed changes

src/taxonomy/TaxonomyLayout.jsx Outdated Show resolved Hide resolved

src/taxonomy/taxonomy-card/TaxonomyCardMenu.jsx Show resolved Hide resolved

xitij2000 reviewed Oct 31, 2023

View reviewed changes

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch 2 times, most recently from d408b52 to a12f262 Compare October 31, 2023 18:45

renovate bot added 3 commits November 6, 2023 13:26

fix(deps): update dependency react-transition-group to v4.4.5 (opened…

d5cc567

…x#647) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

fix(deps): update dependency @edx/frontend-component-footer to v12.5.1 (

09f908b

openedx#650) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update dependency @edx/frontend-build to v13.0.4 (opened…

f61f742

…x#641) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

pomegranited approved these changes Nov 6, 2023

View reviewed changes

fix: end date error when certificate row not shown (openedx#668)

02a683f

KristinAoki and others added 2 commits November 7, 2023 14:05

fix: fidelity typo in preference api (openedx#662)

05e90b5

fix(deps): update dependency @edx/frontend-component-header to v4.9.3 (…

2cfde7d

…openedx#651) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

renovate bot and others added 4 commits November 7, 2023 15:43

fix: modal exit redirect (openedx#659)

3dac6aa

refactor: files-and-videos folder (openedx#672)

78eb512

feat: remove old/duplicate proctoring component (openedx#671)

7c7b3cd

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 9ae8dc1 to c6b68bf Compare November 11, 2023 13:10

feat: add taxonomy detail page

56dbdd2

rpenido force-pushed the rpenido/fal-3529-bare-bones-taxonomy-detail-page branch from 568ee3e to 56dbdd2 Compare November 11, 2023 15:07

zacharis278 and others added 3 commits November 13, 2023 09:14

feat: opt out is not supported by lti proctoring (openedx#673)

e41efba

This toggle does nothing if an LTI tool is selected. We should hide it in that case.

fix: total file count update on add and delete (openedx#681)

2fbb490

fix: combine filter and sort into one modal (openedx#680)

3378c8e

ChrisChV reviewed Nov 13, 2023

View reviewed changes

ChrisChV approved these changes Nov 13, 2023

View reviewed changes

src/taxonomy/taxonomy-detail/TaxonomyDetailMenu.jsx Outdated Show resolved Hide resolved

pomegranited reviewed Nov 13, 2023

View reviewed changes

src/taxonomy/data/types.mjs Outdated Show resolved Hide resolved

KristinAoki and others added 3 commits November 14, 2023 10:43

fix: active transcript preference not loading (openedx#682)

7c7ea1f

fix: table search filter (openedx#683)

ce9db57

pomegranited reviewed Nov 14, 2023

View reviewed changes

src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx Outdated Show resolved Hide resolved

renovate bot and others added 6 commits November 15, 2023 16:47

fix(deps): update dependency @edx/brand to v1.2.3 (openedx#667)

14e3c25

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update dependency @edx/frontend-build to v13.0.5 (opened…

416ac4f

…x#676) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

chore(deps): update dependency axios-mock-adapter to v1.22.0 (openedx…

2804f38

…#678) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Merge branch 'master' into rpenido/fal-3529-bare-bones-taxonomy-detai…

9c582ed

…l-page

fix: address nits in PR review

9df1699

refactor: move data/selectors to data/apiHooks

0fdaa07

and fix tests to mock useQuery.

pomegranited closed this Nov 16, 2023

pomegranited mentioned this pull request Nov 16, 2023

feat: bare bones taxonomy detail page [FC-0036] openedx/frontend-app-authoring#655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[temp] feat: add detail taxonomy page #5

[temp] feat: add detail taxonomy page #5

rpenido commented Oct 20, 2023 •

edited

Loading

pomegranited left a comment

codecov bot commented Oct 25, 2023 •

edited

Loading

pomegranited left a comment

bradenmacdonald commented Oct 26, 2023

pomegranited left a comment

xitij2000 Oct 31, 2023

rpenido Oct 31, 2023 •

edited

Loading

xitij2000 Oct 31, 2023

rpenido Oct 31, 2023

xitij2000 Oct 31, 2023

rpenido Oct 31, 2023

bradenmacdonald commented Nov 3, 2023

rpenido commented Nov 3, 2023

pomegranited left a comment

bradenmacdonald commented Nov 7, 2023

rpenido commented Nov 7, 2023

ChrisChV Nov 13, 2023

ChrisChV left a comment

pomegranited commented Nov 16, 2023

[temp] feat: add detail taxonomy page #5

[temp] feat: add detail taxonomy page #5

Conversation

rpenido commented Oct 20, 2023 • edited Loading

Supporting Information

pomegranited left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

pomegranited left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Oct 26, 2023

pomegranited left a comment

Choose a reason for hiding this comment

xitij2000 Oct 31, 2023

Choose a reason for hiding this comment

rpenido Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

xitij2000 Oct 31, 2023

Choose a reason for hiding this comment

rpenido Oct 31, 2023

Choose a reason for hiding this comment

xitij2000 Oct 31, 2023

Choose a reason for hiding this comment

rpenido Oct 31, 2023

Choose a reason for hiding this comment

bradenmacdonald commented Nov 3, 2023

rpenido commented Nov 3, 2023

pomegranited left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Nov 7, 2023

rpenido commented Nov 7, 2023

ChrisChV Nov 13, 2023

Choose a reason for hiding this comment

ChrisChV left a comment

Choose a reason for hiding this comment

pomegranited commented Nov 16, 2023

rpenido commented Oct 20, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

rpenido Oct 31, 2023 •

edited

Loading